fix(server): centralize pre-validation hooks and attribute chain failures#938
Merged
Conversation
…ures Move the pre-validation hook types, compose_pre_validation_hooks, and the _flatten/_apply helpers into a single source-of-truth module adcp.server._hooks. spec_compat.py re-imports and re-exports the public names with __all__ so existing imports keep working; __init__.py, a2a_server.py, mcp_tools.py, and serve.py now import from _hooks. Add PreValidationHookError so INVALID_REQUEST messages name the failing chain index and callable (e.g. "pre_validation_hook[1] second_hook raised RuntimeError: ...") instead of a generic "pre_validation_hook raised ...". The dispatcher catches it on the shared create_tool_caller path, so both MCP and A2A surface the attributed message. Replace the mock/spy MCPToolSet test with a real call_tool() behavior test, add an A2A ordered-chain executor test, and add MCP + A2A chain-failure attribution tests asserting the index and callable appear in the error. Closes #859 Supersedes #882 Co-Authored-By: sangilish <sangilish@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![aao-ipr-bot[bot]](https://avatars.githubusercontent.com/u/134922?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean refactor. Pulls the pre-validation hook machinery into one stdlib-only _hooks.py and lets it sit at the bottom of the server import layering — single source of truth, every transport reads the same code path.
Things I checked
_applysemantics are equivalent + better._flatten_pre_validation_hooksis byte-identical to the original. New_apply_pre_validation_hooks(_hooks.py:73-99) wraps every hook exception and every non-dict return inPreValidationHookErrorinstead of letting them escape raw / raising bareTypeError. SameINVALID_REQUESTcode on the wire, strictly better message. Happy path unchanged.- Narrowing
except Exception→except PreValidationHookError(mcp_tools.py:2333-2342) is safe._flatten(the only thing that raisesTypeErrorfor a non-callable chain) runs atmcp_tools.py:2327, outside the try and at caller-creation time, not on the dispatch path. Inside the try only_applyruns, and_applynow wraps everything intoPreValidationHookError. Nothing else escapes. - No import breaks.
spec_compat.pyre-imports and re-exports the public names with an explicit__all__;Mapping/Sequencedropped (only the deleted funcs used them),Callableretained forspec_compat.py:175,Collectionretained. The un-migrated consumeradcp.testing.decisioning(from adcp.server.spec_compat import PreValidationHooks) still resolves._spec_compat_hooks_implkept fortest_spec_compat_hooks.py. - Import layering holds.
_hooks.pyimports onlycollections.abc/typing— nogenerated_poc/_generatedaccess.test_import_layering.pygoverns the type layer, not this. - Tests exercise the real path. The mock/spy
MCPToolSettest is gone, replaced by a realMCPToolSet.call_tool(...)chain test; the A2A test drivesADCPAgentExecutor.executeend-to-end and asserts the attributed message round-trips through both theadcp_errorenvelope and the mirrored TextPart. Assertions (pre_validation_hook[1],boom,RuntimeError,returned list, expected dict) match thePreValidationHookErrorformat strings exactly. - Semver signal correct. Additive — new
PreValidationHookErrorexport, all prior names re-exported.fix(server):is the right shape; nothing on the public surface removed or re-typed.
Follow-ups (non-blocking — file as issues)
- New public export
PreValidationHookErrorreachesadcp.server.*. CheckAGENTS.md/README.mdfor the hook-error surface so the docs don't drift behind the new attribution behavior.
Minor nits (non-blocking)
- Self-wrapping catch.
_hooks.py:88(except Exception) will re-wrap aPreValidationHookErrorinto a nestedraised PreValidationHookError: ...if an adopter hook ever raises the framework's own type. Harmless — no adopter has reason to — but the catch is a hair broader thanexcept Exceptionminus the framework type.
LGTM. Follow-up noted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Diagnostics + test-quality follow-up to the pre-validation hook composition work that shipped in #860. The ordered-chain core (
PreValidationHook/PreValidationHookChain/compose_pre_validation_hooks) already lives onmain; this PR re-applies the remaining substance from @sangilish's draft #882 freshly onto currentmain(which moved substantially via #930/#933, rewritingserve.py/a2a_server.py/mcp_tools.py).Changes
compose_pre_validation_hooks, and the_flatten/_applyhelpers into a newadcp.server._hooksmodule.spec_compat.pyre-imports and re-exports the public names (with__all__) so existing imports — includingadcp.testing.decisioning'sfrom adcp.server.spec_compat import PreValidationHooks— keep working.__init__.py,a2a_server.py,mcp_tools.py, andserve.pynow import from_hooks.PreValidationHookErrorsoINVALID_REQUESTnames the failing chain index and callable, e.g.pre_validation_hook[1] second_hook raised RuntimeError: ...(andpre_validation_hook[0] bad_return returned list, expected dict) instead of a genericpre_validation_hook raised .... The dispatcher catches it on the sharedcreate_tool_callerpath, so MCP and A2A both surface the attributed message (A2A via theadcp_errorenvelope + mirrored TextPart).MCPToolSettest with a realMCPToolSet.call_tool(...)behavior test; add an A2A ordered-chain executor test; add MCP + A2A chain-failure attribution tests asserting the index and callable appear in the error.Import layering
_hooks.pydepends only on the standard library, so it sits at the bottom of theadcp.serverlayering — every other server module may import from it freely, and it adds nogenerated_poc/_generatedaccess.tests/test_import_layering.pypasses.Verification
tests/test_import_layering.py,tests/test_public_api.py— passtests/test_pre_validation_hooks.py,tests/test_a2a_server.py,tests/test_spec_compat_hooks.py(+ deprecation), unknown-field / schema-validation / serve-passthrough / testing-decisioning — passmake lint,make typecheck,make typecheck-all,make validate-generated— all greenCredit to @sangilish, whose draft #882 this re-applies; that PR can be closed as superseded.
Closes #859
Supersedes #882
🤖 Generated with Claude Code